You're right that a macro just for this impl isn't much use. However I think we should aim for a 1-1 correspondence between ResourceMarkers and keys, because defining ResourceMarker on different structs with the same key seems like a footgun.

I'd say a ResourceKey and its single ResourceMarker impl should be generated by the same macro. Maybe something like the current #[data_struct] that also generates the keys and links them:

#[icu_provider::resource_struct]
#[resource_key(KEY_NAME, "key/path@1")]
#[resource_key(KEY_NAME_2, "key/path_2@1")]
pub struct FooV1<'data> { ... }

or alternatively with more prominent keys:

#[icu_provider::data_struct]
pub struct FooV1<'data> { ... }

define_resource!(KEY_NAME, "key/path@1", FooV1);
define_resource!(KEY_NAME_2, "key/path_2@1", FooV1);

i like this idea

Yeah! We should do something along these lines. I like the first version better. I would also like to get rid of the separate key constants, because you will be able to access them via FooV1Marker::KEY.

Here's another interesting proposal. We can allow only one key per struct, and for structs with multiple keys, we make them newtypes with #[serde(transparent)]. They can be cast to the inner type if needed.

Here's what I got working:

use icu_provider::prelude::*;
use std::borrow::Cow;

// We also need `yoke` in scope: <https://github.com/unicode-org/icu4x/issues/1557>
use icu_provider::yoke;

#[icu_provider::data_struct(
    "demo/foo@1",
    BarV1Marker = "demo/bar@1",
    BazV1Marker = "demo/baz@1"
)]
pub struct FooV1<'data> {
    message: Cow<'data, str>,
};

assert_eq!(FooV1Marker::KEY.get_path(), "demo/foo@1");
assert_eq!(BarV1Marker::KEY.get_path(), "demo/bar@1");
assert_eq!(BazV1Marker::KEY.get_path(), "demo/baz@1");

Cool!

Do we need custom marker names? We could derive them from the key path: "demo/foo@1" -> DemoFooV1Marker (or maybe ignore the name space as the rust module takes that role).

Not a fan of the newtype solution as that would add quite a bit of boilerplate.

I strongly dislike the practice performed by a handful of computer frameworks that auto-generate names by munging other names into a different case convention. I would much rather have the literal symbol DemoFooV1Marker be in source code.

In that case I think it'd be neater if the default marker name wasn't generated from the struct name but was also a required param.

And if we don't use equivalent names, should we at least verify that the key's version matches the struct's version?

Good call. I changed the macro so that the marker type is explicit.

And if we don't use equivalent names, should we at least verify that the key's version matches the struct's version?

Seems unnecessary to me, but we could do it in a follow-up.

i like this idea

Should KeyedDataProvider be KeyedDynProvider now?

Maybe. But I want to get rid of KeyedDataProvider and it is tracked in #442, and it is a private internal type to provider_cldr. So I prefer not to mess with it right now.

I think these functions should be adding one piece of context at a time. If the client wants to log both key and request they can do .with_key(k).with_options(o), but they should be able to only log options as well (we already have key-only). This is useful for

• RequestOptions::try_langid, which can return DataErrorKind::NeedsLocale.with_options(self)
• DataPayload::try_langid which wouldn't have to accept a context: ResourceKey, as that can be added by the caller with with_key

I wanted with_req to require both arguments because they are both important as context for the errors. This is a side-effect of splitting the key and the options into separate arguments.

In that case I think it'd be neater if the default marker name wasn't generated from the struct name but was also a required param.

And if we don't use equivalent names, should we at least verify that the key's version matches the struct's version?

While we're changing the API, I think we should add constructors for ResourceOptions and make the fields private. Currently there is no instance that has a variant but no langid, and I think we should require that. Otherwise we cannot parse stringified ResourceOptions, as both

ResourceOptions { variant: Some(Cow::Borrowed("und")), langid: None}

ResourceOptions { variant: None, langid: Some(langid!("und")) }

are 'serialized' to "und".

We need that for #1092.

As discussed, I don't want to make this change right now because I am reconsidering the whole data model of ResourceOptions as part of #243.

issue: I'm not a huge fan of "type 1" and "type 2" naming, can we just call these any providers and buffered providers?

#1576 (comment)